-
Notifications
You must be signed in to change notification settings - Fork 628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Django ASGI support #391
Conversation
c605154
to
5cf5b16
Compare
Marking this as ready for review. There are still a few things to investigate:
cc @lonewolf3739, because of the context provided in #334. |
@adamantike is this still WIP? If so, please mark it as draft. |
Oh, I see, please edit the PR description instead. |
...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py
Show resolved
Hide resolved
.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py
Outdated
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py
Outdated
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py
Outdated
Show resolved
Hide resolved
I have rebased, fixed some comments, and added support for traced request attributes! The only issue I still find is this one:
However, it could be related to how Pytest handles ContextVars and contexts related to attached tokens, based on pytest-dev/pytest-asyncio#127 |
d7956d5
to
f33b9cd
Compare
Please do not merge yet. I just found that the I will need to debug what's going on. I'm open to any insights about what could be generating this issue. |
I have filed https://code.djangoproject.com/ticket/32815, to determine if the bug is in Django's codebase instead. |
New-style middlewares were [introduced][django_1_10_changelog] in Django `1.10`, and also `settings.MIDDLEWARE_CLASSES` was [removed][django_2_0_changelog] in Django 2.0. This change migrates the Django middleware to conform with the new style. This is useful because it will help us solve the pending issue in open-telemetry#391. By having a single entrypoint to the middleware, `__call__`, which is [wrapped with `sync_to_async` just once][call_wrapped] for async requests, we avoid the [issue][asgiref_issue] where a `ContextVar` cannot be reset from a different context. With the current deprecated `MiddlewareMixin` way, both `process_request` and `process_response` were being [wrapped separately with `sync_to_async`][mixin_wrapped], which was the source of the mentioned issue. [django_1_10_changelog]: https://docs.djangoproject.com/en/3.2/releases/1.10/#new-style-middleware [django_2_0_changelog]: https://docs.djangoproject.com/en/3.2/releases/2.0/#features-removed-in-2-0 [call_wrapped]: https://github.com/django/django/blob/213850b4b9641bdcb714172999725ec9aa9c9e84/django/core/handlers/base.py#L54-L57 [mixin_wrapped]: https://github.com/django/django/blob/213850b4b9641bdcb714172999725ec9aa9c9e84/django/utils/deprecation.py#L137-L147 [asgiref_issue]: django/asgiref#267
New-style middlewares were [introduced][django_1_10_changelog] in Django `1.10`, and also `settings.MIDDLEWARE_CLASSES` was [removed][django_2_0_changelog] in Django 2.0. This change migrates the Django middleware to conform with the new style. This is useful because it will help us solve the pending issue in open-telemetry#391. By having a single entrypoint to the middleware, `__call__`, which is [wrapped with `sync_to_async` just once][call_wrapped] for async requests, we avoid the [issue][asgiref_issue] where a `ContextVar` cannot be reset from a different context. With the current deprecated `MiddlewareMixin` way, both `process_request` and `process_response` were being [wrapped separately with `sync_to_async`][mixin_wrapped], which was the source of the mentioned issue. [django_1_10_changelog]: https://docs.djangoproject.com/en/3.2/releases/1.10/#new-style-middleware [django_2_0_changelog]: https://docs.djangoproject.com/en/3.2/releases/2.0/#features-removed-in-2-0 [call_wrapped]: https://github.com/django/django/blob/213850b4b9641bdcb714172999725ec9aa9c9e84/django/core/handlers/base.py#L54-L57 [mixin_wrapped]: https://github.com/django/django/blob/213850b4b9641bdcb714172999725ec9aa9c9e84/django/utils/deprecation.py#L137-L147 [asgiref_issue]: django/asgiref#267
Filed #533, which circumvents the |
Rebased now that #533 was merged. All the |
@@ -45,6 +45,8 @@ install_requires = | |||
opentelemetry-semantic-conventions == 0.23.dev0 | |||
|
|||
[options.extras_require] | |||
asgi = | |||
opentelemetry-instrumentation-asgi == 0.23.dev0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be documented so people using ASGI with Django realize they need to install these extra deps to make it work.
How do ASGI deps work with Django? Do regular Django projects need to install additional packages to make ASGI work or does Django now ship with (or depend on) bundled ASGI module. If some level of ASGI support is always present in Django now, may be we can add this as a main dependency instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good discussion topic. Django doesn't require any additional/extra dependencies for ASGI to work (aside from running it using Uvicorn/Daphne), and bundles asgiref
since the async introduction in Django 3.0: django/django@a415ce7#diff-60f61ab7a8d1910d86d9fda2261620314edcae5894d5aaa236b821c7256badd7
This dependency doesn't include other heavy dependencies that aren't already required by opentelemetry-instrumentation-wsgi
. However, we also have to consider that ASGI is only present since Django 3.x, while this project tries to maintain support for older Django versions.
I think the only missing change on this PR, if we go with adding this to install_requires
, is to make _is_asgi_supported
also depend on DJANGO_3_0
being True
, as we won't receive any ImportError
anymore when importing ASGI dependencies.
.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py
Show resolved
Hide resolved
.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py
Show resolved
Hide resolved
@@ -141,12 +166,23 @@ def process_request(self, request): | |||
if self._excluded_urls.url_disabled(request.build_absolute_uri("?")): | |||
return | |||
|
|||
is_asgi_request = _is_asgi_request(request) | |||
if is_asgi_request and not _is_asgi_supported: | |||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can a django service/middleware handle both WSGI and ASGI requests at the same time? If it can only handle one kind, may be this can can run in middleware init and result can be stored in an instance variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least is_asgi_supported can be called before _is_asgi_request and that way we can avoid running the type check for non-ASGI requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can a django service/middleware handle both WSGI and ASGI requests at the same time?
Yes, it can handle both, so we need to check which kind of request we're receiving on each call.
At least is_asgi_supported can be called before _is_asgi_request and that way we can avoid running the type check for non-ASGI requests.
As we're using is_asgi_request
multiple times on this method, I don't think we can avoid the type-check call.
- If ASGI is not supported, we need to know if the current request is ASGI.
- If ASGI is supported, we need to know it nevertheless, to decide on which carrier, carrier_getter, etc. to use.
if is_asgi_request: | ||
attributes = collect_request_attributes(request.scope) | ||
else: | ||
attributes = collect_request_attributes(request_meta) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also set a variable named carrier
to request.scope
or request_meta
in the similar is_asgi_request check above and here just call the function on carrier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, applied!
# ASGI requests include extra attributes in request.scope.headers. For this reason, | ||
# we need to build an object with the union of `request` and `request.scope.headers` | ||
# contents, for the extract_attributes_from_object function to be able to retrieve | ||
# attributes from it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't have to use extract_attribute_from_object
. I think it'd be simpler to just write a dedicated extract function for Django ASGIRequest objects instead of doing all this just to be able to use the existing func.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make more sense, to keep using the extract_attributes_from_object
util, to go with a composition approach?
attributes = extract_attributes_from_object(
request, self._traced_request_attrs, attributes
)
if is_asgi_request:
# ASGI requests include extra attributes in request.scope.headers.
attributes = extract_attributes_from_object(
types.SimpleNamespace(**{
name.decode("latin1"): value.decode("latin1")
for name, value in request.scope.get("headers", [])
}),
self._traced_request_attrs,
attributes,
)
"{} {}".format(response.status_code, response.reason_phrase), | ||
response, | ||
) | ||
if is_asgi_request: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does add_response_attributes
only set a status code? The name suggests it does more than that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does actually. It adds HTTP status code attribute in addition to setting status on the span. Plus is also has some additional logic which we miss out on here. Any reason we cannot just use that function for ASGI as well?
Lines 188 to 208 in e129174
def add_response_attributes( | |
span, start_response_status, response_headers | |
): # pylint: disable=unused-argument | |
"""Adds HTTP response attributes to span using the arguments | |
passed to a PEP3333-conforming start_response callable.""" | |
if not span.is_recording(): | |
return | |
status_code, _ = start_response_status.split(" ", 1) | |
try: | |
status_code = int(status_code) | |
except ValueError: | |
span.set_status( | |
Status( | |
StatusCode.ERROR, | |
"Non-integer HTTP status: " + repr(status_code), | |
) | |
) | |
else: | |
span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code) | |
span.set_status(Status(http_status_to_status_code(status_code))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the functions are right now, I don't see any needed changes for both WSGI and ASGI to make the same changes to the current span:
- WSGI:
Lines 188 to 208 in e129174
def add_response_attributes( span, start_response_status, response_headers ): # pylint: disable=unused-argument """Adds HTTP response attributes to span using the arguments passed to a PEP3333-conforming start_response callable.""" if not span.is_recording(): return status_code, _ = start_response_status.split(" ", 1) try: status_code = int(status_code) except ValueError: span.set_status( Status( StatusCode.ERROR, "Non-integer HTTP status: " + repr(status_code), ) ) else: span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code) span.set_status(Status(http_status_to_status_code(status_code))) - ASGI:
Lines 125 to 140 in e129174
def set_status_code(span, status_code): """Adds HTTP response attributes to span using the status_code argument.""" if not span.is_recording(): return try: status_code = int(status_code) except ValueError: span.set_status( Status( StatusCode.ERROR, "Non-integer HTTP status: " + repr(status_code), ) ) else: span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code) span.set_status(Status(http_status_to_status_code(status_code)))
response, | ||
) | ||
if is_asgi_request: | ||
set_status_code(span, response.status_code) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTTP status codes need to be translated to OpenTelemetry status codes before setting the span status e.g,
Line 208 in e129174
span.set_status(Status(http_status_to_status_code(status_code))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think this is already fixed in the current set_status_code
implementation.
@adamantike Hi, thanks for taking this up. Are you still planning to finish this? |
@owais, I'll be tackling your comments this week, so we can move forward! Thanks for all the feedback |
6b30587
to
cae7e53
Compare
ef98549
to
44cd1dc
Compare
@adamantike sorry for the delay. can you please resolve the conflicts and rebase/merge with main? We can try to include it in the release this week. |
This diff adds `asgi` as an extra, and uses its methods if the current request is an `ASGIRequest`. I still need to dig deeper in the current test suite, to find a way to duplicate the tests in `instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py`, but using an [`AsyncClient`](https://docs.djangoproject.com/en/3.1/topics/testing/tools/#testing-asynchronous-code). Fixes open-telemetry#165, open-telemetry#185, open-telemetry#280, open-telemetry#334.
Add support for the `extract_attributes_from_object` function to retrieve traced attributes that are added to `request.scope.headers`. This is added based on the code found in [Django's `AsyncRequestFactory`]( https://github.com/django/django/blob/a948d9df394aafded78d72b1daa785a0abfeab48/django/test/client.py#L550-L553), which indicates that any extra argument sent to the `AsyncClient` is set in the `request.scope.headers` list. Also: * Stop inheriting from `SimpleTestCase` in async Django tests, to simplify test inheritance. * Correctly configure Django settings in tests, before calling parent's `setUpClass` method. * Rebase and port the latest changes to Django WSGI tests, to be supported by ASGI ones.
44cd1dc
to
7b82acb
Compare
No problem! I have rebased and resolved existing conflicts. |
extra_requires sounds better to me if we are pulling in deps that non-asgi users won't need. |
I agree, though by that same approach, a future breaking change should be introduced for This is where pypa/setuptools#1139 would come useful if implemented. If no extras are specified, you get both WSGI and ASGI. If you want to cut down on dependencies, you need to specify which flow your app is going to use. |
Oh I've been waiting on pypa/setuptools#1139 for longer than I remember. It's such an obvious feature to have :) |
Description
This diff adds
asgi
as an extra, and uses its methods if the current request is anASGIRequest
. Also adds support for traced request attributes, by retrieving them from request scope headers.Fixes #165, fixes #185, fixes #280, fixes #334.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
tox -e test-instrumentation-django -- -k TestMiddlewareAsgi
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.